diagnostics: shorten paths of unique symbols #73996

da-x · 2020-07-03T14:52:01Z

This is a step towards implementing a fix for #50310, and continuation of the discussion in Pre-RFC: Nicer Types In Diagnostics - compiler - Rust Internals. Impressed upon me from previous discussion in #21934 that an RFC for this is not needed, and I should just come up with code.

The recent improvements to use suggestions that I've contributed have given rise to this implementation. Contrary to previous suggestions, it's rather simple logic, and I believe it only reduces the amount of cognitive load that a developer would need when reading type errors.

If a symbol name can only be imported from one place, and as long as it was not glob-imported anywhere in the current crate, we can trim its printed path to the last component.

This has wide implications on error messages with types, for example, shortening std::vec::Vec to just Vec, as long as there is no other Vec importable from anywhere.

rust-highfive · 2020-07-03T14:52:05Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2020-07-03T15:18:30Z

This is an ingenious solution that as shown can get us quite far and love it's simplicity. I'm concerned about two things:

the memory implications of this change on larger projects, but the Symbols are interned so this shouldn't be a huge impact and is at worst linear
now we use the name only, even if the type is not in scope

This last point has a couple of implications. Types that haven't been imported might produce errors that don't let you figure out how to suggest them. Also I didn't see the whole PR yet but there are use suggestions that shouldn't be affected.

All in all, I still need to review but I feel like this might be a good stopgap solution to this problem until we finally implement a more "principled" solution that takes into account the scope the error is emitted from.

Cc @rust-lang/compiler

da-x · 2020-07-03T15:54:37Z

Thanks for looking into this! Several things -

If we limit this to prelude imports then Vec benefits but std::collections::HashMap doesn't, which IMHO does not look too well.

Adding scope checks to check, for example, whether std::collections::HashMap is imported at the error site has some drawbacks.

I think there's some value in having consistency across all the type errors of a single crate build invocation, considering that we can have one module do use std::collections::HashMap and another not have the import statement.
Whether HashMap import is actually used or not is not tied entirely to whether type errors would refer to HashMap at the scope in question, and I believe developers would like to avoiding unused imports just for the sake of shorter types being printed.

As it is now in the PR, it is now does not affect use statements. If we short paths for non-prelude types like HashMap then newcomers might guess wrong that it's in the prelude, but that's okay because the compiler then just tells them to use std::collections::HashMap via the existing use suggestions. That's why the PR only does this for importable items - because the use suggestions backs them up. It's an extra step. Maybe it's not enough?

estebank · 2020-07-03T17:22:28Z

The point about consistency doesn't sway me much as I would like people to be able to copy things from the output and paste them where the error happened and have things just work, but the later point makes a lot of sense: people will just get the follow up suggestion for imports. This last thing is something we already rely on other cases and I feel is a perfectly fine workflow. I'll review today but now I think this is a perfectly reasonable approach. It will still obviously not address the case of repeated identifiers but it's already a huge improvement.

src/test/ui/associated-type-bounds/assoc-type-eq-with-dyn-atb-fail.stderr

src/librustc_resolve/lib.rs

petrochenkov · 2020-07-04T05:35:22Z

Could you also run perf on this before merging?

This PR doesn't do too much harm, so I guess it's ok for it to exist until something more principled is implemented.
(Didn't look at the details.)

da-x · 2020-07-04T06:56:09Z

Going over the long list of test changes (~1250 files!), found some bits to work out:

~~The String-related hints in src/test/ui/span/issue-39018.stderr are gone.~~ (fixed)
~~std::fmt::Display doesn't get shortened. Likely collides with std::path::Display, so shortening is avoided because of it.~~(fixed)
~~std::result::Result doesn't get shortened. Colliding with std::io::Result? This being a sister type of Option, which does get handled, it's a bit unfortunate.~~ (fixed)
~~There are still a few places where std::marker::Copy is not shortened without a clear reason why.~~ (fixed)
Sometimes, but not always, rustc stage 1 link fails with error: cannot link together two panic runtimes: panic_abort and panic_unwind. I suspect this is related to the lookup_import_candidates doing its Rust 2018-related section on the good path, even when there are no errors, fully adding both panic crates into the resolver when normally it would not have happened. (fixed)

estebank · 2020-07-06T21:41:15Z

The String-related hints in src/test/ui/span/issue-39018.stderr are gone.

rustc_on_unimplemented uses the string representation of the path for types to select what message to give. We'll need to change the code in src/librustc_trait_selection/traits/error_reporting/mod.rs:report_selection_error to always use the full path.

src/librustc_middle/ty/context.rs

da-x · 2020-07-07T06:48:38Z

rustc_on_unimplemented uses the string representation of the path for types to select what message to give. We'll need to change the code in src/librustc_trait_selection/traits/error_reporting/mod.rs:report_selection_error to always use the full path.

Thanks for the hint - I got it fixed.

da-x · 2020-07-07T17:11:50Z

@estebank Separating uniqueness for trait symbols and type symbols seems to sort out the overlap between std::fmt::Display and std::path::Display.
Also, allowing type aliases to be superseded in uniqueness by the types of the same name will take care of the std::io::Result vs std::result::Result overlap, with std::result::Result winning for Result.

WDYT?

nikomatsakis · 2020-07-09T21:03:01Z

I've not read the diff, but I'm very excited to see progress on this problem! 💖

estebank · 2020-07-09T23:27:00Z

@estebank Separating uniqueness for trait symbols and type symbols seems to sort out the overlap between std::fmt::Display and std::path::Display.
Also, allowing type aliases to be superseded in uniqueness by the types of the same name will take care of the std::io::Result vs std::result::Result overlap, with std::result::Result winning for Result.

WDYT?

I'm of diverging opinions. I was originally going to dismiss this alternative out of hand as I fear it would be confusing to do this deduplication on a per type basis. But then I thought a bit about it and 1) it makes absolute sense to ignore type aliases (at least for now) because we never refer to them in type errors and 2) we already give a short description of what we are talking about (expected type 'Result', found trait 'Display'), which would make that work out nicely. The only case where I am slightly concerned is when both "expected" and "found" clash: expected type 'Display', found trait 'Display' could be confusing in of itself. Could we have a way to handle that case specifically? I feel that we can do these things as a follow up PR, what do you think?

da-x · 2020-07-10T06:16:38Z

Yes, we can solve for specific ambiguities with later PRs.

I've pushed remaining fixes discussed for Result and Copy.

Going to check the performance of find_unique_symbols, now that lookup_import_candidates is done on the good path, and not only during use suggestions.

Also, should we allow users to disable this with a -Z in case issues arise, to ease debugging in nightly?

estebank · 2020-07-10T17:09:20Z

Also, should we allow users to disable this with a -Z in case issues arise, to ease debugging in nightly?

Yes.

da-x · 2020-07-10T17:30:57Z

Performance -

For rustc invoked under perf with callgraphs (perf record -F 99 --call-graph=dwarf ), when building the main ripgrep crate,find_unique_symbols takes a total of 2.54% of execution time.

-    2.54%     0.00%  rustc  librustc_driver-3de5a075e28cbed1.so  [.] rustc_resolve::diagnostics::<impl rustc_resolve::Resolver>::find_unique_symbols                                                                        ◆
   - rustc_resolve::diagnostics::<impl rustc_resolve::Resolver>::find_unique_symbols                                                                                                                                         ▒
      - 2.50% rustc_resolve::diagnostics::<impl rustc_resolve::Resolver>::lookup_import_candidates (inlined)                                                                                                                 ▒
           rustc_resolve::diagnostics::<impl rustc_resolve::Resolver>::lookup_import_candidates_from_module                                                                                                                  ▒
         - rustc_resolve::ModuleData::for_each_child (inlined)                                                                                                                                                               ▒
            - 2.18% rustc_resolve::Resolver::resolutions                                                                                                                                                                     ▒
               - rustc_resolve::build_reduced_graph::<impl rustc_resolve::Resolver>::build_reduced_graph_external (inlined)                                                                                                  ▒
                  - 1.17% rustc_metadata::rmeta::decoder::cstore_impl::<impl rustc_metadata::creader::CStore>::item_children_untracked                                                                                       ▒
                       rustc_metadata::rmeta::decoder::<impl rustc_metadata::creader::CrateMetadataRef>::each_child_of_item (inlined)                                                                                        ▒
                    1.01% rustc_resolve::build_reduced_graph::BuildReducedGraphVisitor::build_reduced_graph_for_external_crate_res                                                                                           ▒
     0.12%     0.00%  rustc  librustc_driver-3de5a075e28cbed1.so  [.] rustc_resolve::diagnostics::_$LT$impl$u20$rustc_resolve..Resolver$GT$::find_unique_symbols::_$u7b$$u7b$closure$u7d$$u7d$::h583030594edeb63a (inlined)

@petrochenkov

src/librustc_middle/ty/print/pretty.rs

src/librustc_trait_selection/traits/error_reporting/mod.rs

* Backport paritytech/substrate#7381 * Bring back genesis storage build in aura/timestamp To not change spec version, see paritytech/substrate#7686 (comment) * Backport paritytech/substrate#7238 * Backport paritytech/substrate#7395 * Bump impl_version * Fix UI tests and bump trybuild dep See rust-lang/rust#73996 Backports: paritytech/substrate#7764 paritytech/substrate#7656 * Partially backport paritytech/substrate#7838 * Release frame-support with a dep compilation fix * Bump patch level for remaining crates This is done because at the time of writing cargo-unleash does not fully support partial workspace publishing and mixes both local and crates.io versions of the packages, leading to errors in the release check workflow. * Backport paritytech/substrate#7854 ...to fix compilation error when using futures-* v0.3.9. * Adding Changelog entry for patch release Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Benjamin Kampmann <[email protected]>

rust-highfive assigned estebank Jul 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2020

da-x changed the title ~~diagnosis: shorten paths of unique symbols~~ diagnostics: shorten paths of unique symbols Jul 3, 2020

petrochenkov self-assigned this Jul 3, 2020

tesuji reviewed Jul 4, 2020

View reviewed changes

src/test/ui/associated-type-bounds/assoc-type-eq-with-dyn-atb-fail.stderr Outdated Show resolved Hide resolved

petrochenkov reviewed Jul 4, 2020

View reviewed changes

src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved

petrochenkov removed their assignment Jul 4, 2020

estebank reviewed Jul 6, 2020

View reviewed changes

src/librustc_middle/ty/context.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

eddyb mentioned this pull request Jul 13, 2020

The compiler should report publicly exported type names if possible #21934

Open